Require JavaInfo on deps, move scrooge srcjar to srcs #523

johnynek · 2018-06-15T20:21:01Z

since we put the scrooge srcjar into deps, we wind up putting all the srcjars as well as the thrift srcs on the classpaths. This is not what we want.

I am having trouble using this rule in a repo with a very large number of thrifts because the classpaths are getting too large. I will do a run with this and see if it bypasses.

cc @ianoc @ittaiz

johnynek · 2018-06-15T20:44:53Z

while this is still probably good, this did not solve my problem. somehow, the thrift jars are still showing up on the classpath.

ittaiz · 2018-06-16T02:02:55Z

Is there value in reviewing this or do you want to iterate a bit more until you find your solution?

…

On Fri, 15 Jun 2018 at 23:44 P. Oscar Boykin ***@***.***> wrote: while this is still probably good, this did not solve my problem. somehow, the thrift jars are still showing up on the classpath. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#523 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF9KXTbfOtoUnkQRhcr80yCUj2bB9ks5t9BzGgaJpZM4UqGd5> .

johnynek · 2018-06-16T02:28:33Z

it is a short review, not much to it, but I don't think we should merge it if it does not improve matters.

I'll try to write a test that asserts that the thrifts don't end up on the classpath.

Meanwhile: bazelbuild/bazel#5413 seems to let me bypass (at the expense of very long startup times due to the quadratic algorithm that bazel uses to construct the jars for long classpaths).

ittaiz · 2018-06-16T02:42:50Z

I'll leave this to @ianoc since I'm not sure I understand the change (it seems you're just passing the srcjar in a different way)

ianoc · 2018-06-16T15:13:57Z

this change lgtm

ianoc · 2018-06-16T15:28:21Z

twitter_scrooge/twitter_scrooge.bzl

  scala_library(
      name = name,
+      srcs = [srcjar],


does this work? i think the scala_library rule is restricted to only allow actual files in the srcs not targets?

(or does it intelligently realize the output file?... interesting)

it does use the output file here. We could be more precise and just list the output filename. That might actually solve the problem.... Maybe by putting the whole target we are sucking up all the deps.

ittaiz · 2018-06-16T16:31:44Z

Can you do that? Restrict to only files (in the declaration)?

…

On Sat, 16 Jun 2018 at 18:29 ianoc ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In twitter_scrooge/twitter_scrooge.bzl <#523 (comment)> : > scala_library( name = name, + srcs = [srcjar], (or does it intelligently realize the output file?... interesting) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#523 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF7ZbjZf51DYwk3BeS-CQj4IIOQQ5ks5t9SRogaJpZM4UqGd5> .

johnynek · 2018-06-20T02:21:50Z

okay, @ittaiz @ianoc can you take a look?

Basically, I removed all the old crufty hacks we had and just require java providers in all places. With this, we don't mistakenly put a bunch of irrelevant stuff on the path. I added a test that we don't add the thrift jar.

This is a bigger change, but takes us towards removing tech debt, so it seems like a win.

ittaiz

Looks good in general.
Please see some comments inside.

ittaiz · 2018-06-20T02:52:36Z

scala/scala.bzl

@@ -12,6 +12,11 @@ load(
    "@io_bazel_rules_scala//specs2:specs2_junit.bzl",
    _specs2_junit_dependencies = "specs2_junit_dependencies")

+load(


ittaiz · 2018-06-20T02:53:30Z

scala/private/common.bzl

@@ -26,15 +26,12 @@ def _collect_jars_when_dependency_analyzer_is_off(dep_targets):
  runtime_jars = []

  for dep_target in dep_targets:
+    # we require a JavaInfo for dependencies
+    # must use java_import or scala_import if you have raw files


Why keep the “if”? I think you can require the divider on the signature of the attribute and so throw all of the conditionals away

because scrooge puts everything in the deps sadly... we want to fix it, but one step at a time.

scrooge macro puts all the thrift dependencies on the deps. :( we need #510

ittaiz · 2018-06-20T02:56:33Z

test/src/main/scala/scala/test/twitter_scrooge/grep.sh

+#!/bin/sh
+
+if grep -q $1 $2 ; then
+  echo "ERROR: found $1"


Maybe add what was expected? Either in the assertion or in the test name since 6 months from now I won’t remember :)

ittaiz · 2018-06-20T02:57:42Z

scala/private/rule_impls.bzl

-      extra_information=_collect_extra_information(ctx.attr.deps),
+      # unfortunately, we need to see this for scrooge and protobuf to work,
+      # but those are generating srcjar, so they should really come in via srcs
+      extra_information=_collect_extra_information(ctx.attr.deps + ctx.attr.srcs),


I’m sorry but I don’t understand this. Where does scroog euse this?

search for all the extra information in the file. For instance: https://github.com/bazelbuild/rules_scala/blob/master/twitter_scrooge/twitter_scrooge.bzl#L85

Really this is something that should be replaced with #510 but that is staged behind a few layers of tech debt removal (a supported skylark function to compile scala and get a JavaInfo, and a toolchain that sets up all the things we need for scala so we don't have to copy-paste the scala setup for the ctx).

ittaiz · 2018-06-20T03:00:20Z

scala/private/common.bzl

    if JavaInfo in dep_target:
      java_provider = dep_target[JavaInfo]
      current_dep_compile_jars = java_provider.compile_jars
      current_dep_transitive_compile_jars = java_provider.transitive_compile_time_jars
      runtime_jars.append(java_provider.transitive_runtime_jars)
-    else:


Again the whole conditional as above but even if I’m wrong why silently ignore and not fail?

johnynek · 2018-06-20T03:12:20Z

okay, @ittaiz I think I've addressed your comments. Thanks for the quick look!

ittaiz

I’m concerned about the “if”.
Previously we supported http_file, now we don’t but it’s not explicit.
People will not easily know.
For us (Wix) specifically this isn’t a problem but I’m concerned for the wider population.
Wdyt?

ittaiz · 2018-06-20T04:56:00Z

I have a thought,
Wdyt about adding a Boolean attribute until #510 is solved which opts in to this new behavior and once 510 is solved we’ll just enforce this requirement (by explicitly adding required providers)

johnynek · 2018-06-20T05:28:37Z

@ittaiz I’ll try that tomorrow. Maybe the same approach to not generate the java jar when we know we don’t need it.

johnynek · 2018-06-20T19:07:25Z

actually, @ittaiz I'd like to not yet fail and plumb the booleans through.

This will be a nontrivial change to wire that optionality down to the bottom of several collection methods and it is throw away work. Next, several places in the code were already lax in that they probed for expected shape and ignored things that don't match the shape.

Let's steer towards requiring providers at the bazel level and not probe anywhere, directly access dep[JavaInfo].

Can we merge this as is to unblock?

ittaiz

One last suggestion- wdyt about adding at least a bold paragraph to the readme saying we silently ignore jars that have no JavaInfo (for example //jar:file)? We can remove the comment when we require providers

ittaiz · 2018-06-20T20:32:26Z

I approved btw as a way to say that if you feel strongly about not adding the comment to the readme then you can merge as is

johnynek · 2018-06-20T21:40:48Z

okay, @ittaiz one more round... I added comments and went ahead and locked down runtime_deps, which scrooge does not use.

ittaiz · 2018-06-21T03:29:52Z

👍🏽

…

On Thu, 21 Jun 2018 at 1:39 P. Oscar Boykin ***@***.***> wrote: Merged #523 <#523>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#523 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF46MFvQDsIqWK8PMankRZm6d1cF-ks5t-s8igaJpZM4UqGd5> .

move scrooge srcjar to srcs

564e1bc

googlebot added the cla: yes label Jun 15, 2018

ianoc reviewed Jun 16, 2018

View reviewed changes

require JavaInfo, add a test to avoid thrift jars on classpath

ad920a0

johnynek changed the title ~~move scrooge srcjar to srcs~~ Require JavaInfo on deps, move scrooge srcjar to srcs Jun 20, 2018

fix strict build, run lint fix

8d63397

ittaiz reviewed Jun 20, 2018

View reviewed changes

address review comments

90dcb05

ittaiz reviewed Jun 20, 2018

View reviewed changes

johnynek added 3 commits June 20, 2018 08:50

Merge branch 'master' into oscar/put_scrooge_into_src

d1a3d5e

merge master, fix tests

4dc2edd

use JavaInfo directly

2a78628

fix tests, take N

eb50d82

ittaiz approved these changes Jun 20, 2018

View reviewed changes

fix take N+1, update README, make runtime_deps strict

026fdb7

johnynek merged commit afa6403 into master Jun 20, 2018

ittaiz deleted the oscar/put_scrooge_into_src branch July 1, 2018 12:36

iphydf mentioned this pull request Jul 21, 2018

maven_jar dependencies no longer work, as of #523 #565

Open

ittaiz mentioned this pull request Jun 20, 2019

JavaInfo provider should be given for deps #774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require JavaInfo on deps, move scrooge srcjar to srcs #523

Require JavaInfo on deps, move scrooge srcjar to srcs #523

johnynek commented Jun 15, 2018

johnynek commented Jun 15, 2018

ittaiz commented Jun 16, 2018 via email

johnynek commented Jun 16, 2018

ittaiz commented Jun 16, 2018

ianoc commented Jun 16, 2018

ianoc Jun 16, 2018

ianoc Jun 16, 2018

johnynek Jun 16, 2018

ittaiz commented Jun 16, 2018 via email

johnynek commented Jun 20, 2018

ittaiz left a comment

ittaiz Jun 20, 2018

ittaiz Jun 20, 2018

johnynek Jun 20, 2018

johnynek Jun 20, 2018

ittaiz Jun 20, 2018

ittaiz Jun 20, 2018

johnynek Jun 20, 2018

ittaiz Jun 20, 2018

johnynek commented Jun 20, 2018

ittaiz left a comment •

edited

Loading

ittaiz commented Jun 20, 2018

johnynek commented Jun 20, 2018

johnynek commented Jun 20, 2018 •

edited

Loading

ittaiz left a comment

ittaiz commented Jun 20, 2018

johnynek commented Jun 20, 2018

ittaiz commented Jun 21, 2018 via email

Require JavaInfo on deps, move scrooge srcjar to srcs #523

Require JavaInfo on deps, move scrooge srcjar to srcs #523

Conversation

johnynek commented Jun 15, 2018

johnynek commented Jun 15, 2018

ittaiz commented Jun 16, 2018 via email

johnynek commented Jun 16, 2018

ittaiz commented Jun 16, 2018

ianoc commented Jun 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz commented Jun 16, 2018 via email

johnynek commented Jun 20, 2018

ittaiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Jun 20, 2018

ittaiz left a comment • edited Loading

Choose a reason for hiding this comment

ittaiz commented Jun 20, 2018

johnynek commented Jun 20, 2018

johnynek commented Jun 20, 2018 • edited Loading

ittaiz left a comment

Choose a reason for hiding this comment

ittaiz commented Jun 20, 2018

johnynek commented Jun 20, 2018

ittaiz commented Jun 21, 2018 via email

ittaiz left a comment •

edited

Loading

johnynek commented Jun 20, 2018 •

edited

Loading